Skip to content

blockchain: Consistent overflow prevention.#3689

Open
davecgh wants to merge 19 commits intodecred:masterfrom
davecgh:multi_blockchain_consistent_overflow_prevention
Open

blockchain: Consistent overflow prevention.#3689
davecgh wants to merge 19 commits intodecred:masterfrom
davecgh:multi_blockchain_consistent_overflow_prevention

Conversation

@davecgh
Copy link
Copy Markdown
Member

@davecgh davecgh commented May 6, 2026

This requires #3677 and is rebased on #3679, #3680, and #3688.

Currently, all overflow detection is done inline in multiple places throughout the blockchain code. It is more ergonomic, consistent, and less error prone to instead use well-tested funcs dedicated to that purpose instead.

Further, consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. In general, unsigned types for values that can never be negative should always be preferred.

Some of the current code does not adhere to that practice and uses plain ints which means the maximum possible value technically changes depending on the architecture.

While these are not currently an issue due to a combination of various limits making it impossible to get anywhere near the limits of the smallest supported architecture (32-bit) and overflow detection, these types of hidden assumptions can easily lead to bugs over time as new features are introduced.

To that end, this consists of a series of commits that add two new functions for adding arguments with a returned flag that indicates whether the result is safe to use (that is no overflow or underflow occurred), convert the existing code to use the new funcs, and change some of the important counting from plain ints to fixed-sized unsigned ints.

Each commit is a self-contained and logically easy to follow change such that the code continues to compile and works properly at each step.

See the description of each commit for further details.

@davecgh davecgh added this to the 2.2.0 milestone May 6, 2026
@davecgh davecgh force-pushed the multi_blockchain_consistent_overflow_prevention branch 2 times, most recently from 444db38 to 6f5c769 Compare May 6, 2026 13:34
davecgh added 10 commits May 6, 2026 17:44
This reworks the tests in TestTreasuryIsFunctions for the treasury add,
treasurybase, and treasury spend identification funcs to make them more
comprehensive, correct some that weren't actually testing what they
claimed, and make them much more consistent with the other tests
throughout the code base.  Not only does it perform more comprehensive
testing, it reduces the test code by about 42%.

In particular:

- Use hex to bytes for hard-coded byte slices for some of the globals
  instead of the much more verbose raw byte slices
- Introduce helper functions to create the various components of the
  transactions
- Start with well-formed transactions and modify them for each test
  instead of building them from scratch every time
- Run all identification funcs against all of the transaction types to
  help ensure none of them are incorrectly detected as any other
- Significantly improves readability and adds descriptions to make it
  clear for people not familiar with the code
- Modernize the test formatting
- Effectively add more tests overall due to cross testing
- Correct test intending to pass stakebase but not treasury add and
  assert it actually passes the stakebase checks

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
Now that the updated treasury spend tests cover the fully valid case,
there is no benefit to repeating it in another test.

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This reworks the treasury spend error tests to use the newly introduced
functions that start with a valid treasury spend and then mutates a copy
to induce the specific error to test.  In the process, it also corrects
some tests that weren't actually tsting what they claimed.

The result is significantly more readable, provides more comprehensive
test coverage, is more consistent with the other tests throughout the
code base, and reduces the test code for the relevant tests by about
69%.

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This reworks the treasury add error tests to use the newly introduced
functions that start with a valid treasury add transaction and then
mutates a copy to induce the specific error to test.  In the process, it
also corrects some tests that weren't actually tsting what they claimed.

The result is significantly more readable, provides more comprehensive
test coverage, is more consistent with the other tests throughout the
code base, and reduces the test code for the relevant tests by about
56%.

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This reworks the treasurybase error tests to use the newly introduced
functions that start with a valid treasurybase and then mutates a copy
to induce the specific error to test.  In the process, it also corrects
some tests that weren't actually tsting what they claimed.

The result is significantly more readable, provides more comprehensive
test coverage, is more consistent with the other tests throughout the
code base, and reduces the test code for the relevant tests by about
63%.

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTAdd method to make it much more consistent with
the other code used in consensus throughout the rest of the code base.

While there are no known exploitable issues with the func and it has
worked well for a while now, it is highly inconsistent with the rest of
the consensus code in style and polish and has various other issues.

For example:

- several of the reported error message are incorrect
- most of the error message don't provide very helpful messages and
  reference internal names that are not visible to users
- inconsistent variable names
- uses less efficient inverted logic tests
- various misleading and inaccurate comments
- exported func comment refers to internal func that is not visible in
  generated documention

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTSpend method to make it much more consistent
with the other code used in consensus throughout the rest of the code
base.

While there are no known exploitable issues with the func and it has
worked well for a while now, it is highly inconsistent with the rest of
the consensus code in style and polish and has various other issues.

For example:

- several of the reported error message are incorrect
- most of the error message don't provide very helpful messages and
  reference internal names that are not visible to users
- inconsistent errors
- inconsistent variable names
- uses less efficient and harder to read inverted logic tests
- various misleading and inaccurate comments

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTreasuryBase method to make it much more
consistent with the other code used in consensus throughout the rest of
the code base.

While there are no known exploitable issues with the func and it has
worked well for a while now, it is highly inconsistent with the rest of
the consensus code in style and polish and has various other issues.

For example:

- several of the reported error message are incorrect
- most of the error message don't provide very helpful messages and
  reference internal names that are not visible to users
- inconsistent variable names
- some checks are not in the most logical order
- various misleading and inaccurate comments
- some checks are not making use of existing funcs

This is part of a larger overall effort to bring the treasury code up to
the standards used throughout the rest of the blockchain consensus code.
This performs some light cleanup of the checkProofOfStake function to
make it more consistent with the rest of the code make the error
messages more accurate and useful.
@davecgh davecgh force-pushed the multi_blockchain_consistent_overflow_prevention branch from 6f5c769 to caac2a1 Compare May 6, 2026 22:47
Comment thread internal/blockchain/checkedmath.go Outdated
Comment thread internal/blockchain/validate.go Outdated
davecgh added 9 commits May 6, 2026 18:08
Currently, all overflow detection is done inline in multiple places
throughout the blockchain code.  It would be more ergonomic, consistent,
and less error prone to instead use well-tested funcs dedicated to that
purpose.

To pave the way, this adds two new functions for adding arguments with a
returned flag that indicates whether the result is safe to use (that is
no overflow or underflow occurred).

One variant is for unsigned ints (uint16, uint32, and uint64) and the
other is for signed ints (int16, int32, and int64).

It only introduces the funcs and does not modify any code to use them.
This adds comprehensive tests for the new addUnsigned and addSigned
funcs for all supported types.
Consensus code should ideally always use types of a specific size so
there is no possibility of divergent behavior when compiled on different
architectures due to differing upper bounds.  Further, unsigned types
for values that can never be negative should always be preferred.

The current signature operations counting code does not adhere to that
practice and uses plain ints which means the maximum possible value
technically changes depending on the architecture.

While this is not currently an issue due to a combination of various
limits making it impossible to get anywhere near the limits of the
smallest supported architecture (32-bit) and overflow detection, these
types of hidden assumptions can easily lead to bugs over time as new
features are introduced.

This remedies that by modifying the consensus level signature operation
counting to use an fixed unsigned 32-bit integer instead of an
architecture dependent signed integer.

Ideally, the return types on the underlying counting funcs in the script
engine would be updated to match and avoid the need to cast, but that
would require a major API version bump since it is a public module, so
this limits the changes to the internal blockchain, mempool, and mining
packages.

While here, it also switches to the new consolidated add funcs with
cleaner overflow detection versus the current more ad-hoc inline
detection.

Note that there is no risk of consensus divergence due to the
aforementioned impossibility of hitting the conditions with the current
combination of parameters and limits.
As mentioned in the previous commit, consensus code should ideally
always use types of a specific size so there is no possibility of
divergent behavior when compiled on different architectures due to
differing upper bounds.  Further, unsigned types for values that can
never be negative should always be preferred.

The current code for counting treasury spend votes does not adhere to
practice and uses plain signed integers.

It is worth noting that this is not an active issue at the moment
because the maximum possible counts achievable due to other limits
imposed on the max number of votes and the window over which the number
of votes are counted are less than the maximum value of the smallest
supported architecture (32 bits).

Nevertheless, these types of hidden assumptions are fragile and can
easily lead to undetected and unexpected behavior when parameters
change and new features are introduced.

The primary goal of this commit is to address that by modifying the
relevant code to use fixed unsigned 32-bit integers instead.

While here, it also takes the opportunity to cleanup the code to make it
more consistent with the other consensus code.

Note that there is no risk of consensus divergence due to the
aforementioned impossibility of hitting the conditions with the current
combination of parameters and limits.
The requirement that treasurybases have zero fee is currently indirectly
enforced by ensuring the input sum is the required subsidy and that the
total of all fees paid in the stake tree do not exceed the input sum.

While that doesn't cause any real issues, it's not very clear and it
ideally should be done in the per-transaction input checks so it is
consistent with all other transaction types.

This modifes CheckTransactionInputs to apply the additional input versus
output sum check to apply to treasurybase transactions as well.
Currently, enforcement that treasury spends commit to the amount they
are spending in the first output and that the amount matches the value
specified as the input amount in the first input happen when blocks are
connected.

The check is not dependent on the current treasury balance or anything
that would require it to be limited to block connection only.  It should
ideally be in the per-transaction input checks so that it is also
enforced early when a treasury spend is added to the mempool.

To accomplish that, this moves that check, along with a couple of other
additional sanity checks, into a separate method dedicated to checking
treasury spend inputs so that it is more consistent with the other stake
transaction type handling and invokes that method from
CheckTransactionInputs.

It also moves the other treasury spend checks after the call that checks
and connects transactions in the stake tree to ensure the commitment is
still verified prior to the other checks that depend on it.
This adds a couple new tests to help ensure the treasurybase overall
amount sum semantics are correct.
The current code recalculates the total stake tree fees via a separate
getStakeTreeFees function in order to pass them to the regular tree
transaction and connection checks so they are accumulated as part of the
total overall fees.

This behavior is correct, but the total fees are already calculated when
checking and connecting the stake tree transactions just before that, so
there is no reason to calculate them again when they can simply be
returned to the caller.

This modifies checkTransactionsAndConnect accordingly and removes the
separate method which is no longer necessary.
This updates the transaction input and fee summing code to make use of
the new consolidated add funcs with cleaner overflow detection.

It also consolidates the input summing for all transaction types into a
new closure instead of repeating the logic multiple times throughout the
input checks function.  Consolidating it makes it more readable and less
error prone.

Finally, while here, it consolidates, cleans up and slightly optimizes
the input sum handling for the transaction types that do not have normal
inputs.

Namely, first the stakebase summing is changes to use the input value
field instead of recalculating the subsidy to make it consistent with
the treasurybase and treasury spend cases.  This is safe because the
code earlier in the function ensures the values matches the expected
amounts.  Second, it combines the checks for all three types since that
change makes the checks for them identical.
@davecgh davecgh force-pushed the multi_blockchain_consistent_overflow_prevention branch from caac2a1 to 0598f52 Compare May 6, 2026 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant